Skip to content

[MINOR][CI] Stop unrelated PR comments from cancelling Delta and ANSI runs - #12906

Merged
jackylee-ch merged 7 commits into
apache:mainfrom
LuciferYang:delta-ut-comment-concurrency
Aug 28, 2026
Merged

[MINOR][CI] Stop unrelated PR comments from cancelling Delta and ANSI runs#12906
jackylee-ch merged 7 commits into
apache:mainfrom
LuciferYang:delta-ut-comment-concurrency

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

delta_spark_ut.yml triggers on pull_request and on issue_comment: types: [created], which fires for every comment posted on an issue or a pull request (review comments on the diff and commit comments use other events). For an issue_comment event github.event.issue.number is that issue's number, which for a comment on a PR is the PR's own number, so the comment run lands in the same concurrency group as that PR's in-flight pull_request run and cancel-in-progress: true kills it. The comment run then does not replace it: delta-test-requested skips anything that is not a /delta-test command, so the suite is lost and nothing takes its place.

The same shape is in velox_backend_ansi.yml: its group is ${{ github.repository }}-ansi-${{ github.event.issue.number || inputs.pr_number }} with cancel-in-progress: true, and its command test lives in jobs.check-comment.if, which is again evaluated after the group. Both files are fixed here the same way.

Fixes #12908.

Neither case is hypothetical. On #12902 I posted an unrelated review comment at 03:37:39Z. It started issue_comment run 33036933287 at 03:37:43Z, which cancelled pull_request run 33034774777 forty-three minutes into the tests. From shard 7's log:

2026-08-27T03:37:41.4970618Z [info] - merge-metrics: insert-only - Partitioned = true, CDF = false
2026-08-27T03:37:43.9917792Z context canceled
2026-08-27T03:37:44.0351639Z ##[error]The operation was canceled.

All eight shards and the aggregate job died within thirty seconds of each other, and the run that killed them concluded skipped. The signal lost was a real one: every failing test in that shard up to the cancellation was in the baseline at the time, so nothing had regressed and the shard was on course to pass.

For ANSI, @felipepessoto reproduced it deliberately on this PR while reviewing it: a /ansi-test comment at 05:38:58Z started run 33043165559, an unrelated follow-up comment at 05:39:40Z created run 33043202388 in the same group two seconds later, and the original run's build-native-lib was cancelled at 05:39:59Z while every job in the replacement was skipped. #12908 has the full trace.

The fix appends a discriminator to the group for that case only. When the comment is not a command the group gains a -comment-<run_id> suffix and the run therefore neither cancels nor queues; for every other trigger the appended segment evaluates to the empty string, so the group name stays byte-identical to what it is today and cancel-in-progress stays true. workflow_dispatch keeps its group too, so a manual ANSI re-dispatch still supersedes an older one for the same PR, which is one of the behaviours #12908 asked to preserve.

In each file the negated condition is the gate job's if repeated verbatim: delta-test-requested for Delta, check-comment for ANSI. One difference between the two is worth naming: check-comment tests the body with contains, not a first-token match, so a comment that merely mentions /ansi-test counts as a command. Copying the condition verbatim keeps the grouping and the gate in agreement on that, rather than making them disagree in a new way. Whether the contains test itself should be tightened is a separate question and not changed here.

Two alternatives I discarded, both of which I had implemented before settling here, in case a reviewer wonders why the expression is shaped this way.

Making cancel-in-progress conditional instead leaves the group alone but does not let the comment run proceed: runs in one group never overlap, so it sits pending until the whole suite finishes, then starts and skips anyway. It also occupies the group's single pending slot in the meantime.

Emitting a -shared suffix for the normal cases rather than an empty one reads better but renames the group for every trigger. pull_request runs use the workflow file from the PR head while issue_comment runs always use the default branch's, so for PRs already open at merge time a push run and a /delta-test run would sit in differently-named groups and stop superseding each other until the PR picks the change up. Appending nothing in those cases avoids the window entirely.

How was this patch tested?

The behaviour cannot be exercised from this PR. Per the issue_comment docs, "This event will only trigger a workflow run if the workflow file exists on the default branch", with GITHUB_REF set to the default branch, so a comment posted here runs main's copy of the workflow no matter what this branch says. That is also why the run that cancelled #12902's suite used the unfixed version. It takes effect once merged.

Three properties are worth checking mechanically rather than by eye, and I checked them for both files by parsing the YAML rather than reading it:

property result
file parses, concurrency.cancel-in-progress true, unchanged from main
group with the appended segment removed byte-identical to main's group
text inside !( ... ) vs the gate job's if equal after collapsing whitespace

Resolved group per trigger, worked through by hand from the precedence rules (&& binds tighter than ||, and both yield values): pull_request, workflow_dispatch and a real command comment all take the || '' branch and keep today's group; any other comment takes format('-comment-{0}', github.run_id). A comment on a plain issue also gets the suffix, since github.event.issue.pull_request is absent, which is harmless because such a run skips too.

I cannot run GitHub's expression evaluator locally, so evaluation itself rests on the docs plus the fact that each file's gate job already evaluates the identical condition on every comment. A malformed expression fails the run with a workflow syntax error rather than mis-grouping silently. If it evaluated to the wrong branch for pull_request events, the symptom would be a group suffixed with -comment-<id>, visible in the Actions UI, and the cost would be pushes no longer superseding older runs rather than a wrong result.

The Delta suite is green on this head: run 33148690783 on 6eec515cd, all eight shards plus the aggregate. Earlier heads of this branch showed four red shards for a reason unrelated to the change, recorded here so the history reads correctly: known-failures.txt listed 32 column-mapping tests as expected failures that had started passing, and fail_on_fixed resolves to true for pull_request events by design, so the gate reported them as NOW-PASSING. @felipepessoto removed exactly those 32 entries on main in #12907, so this branch does not touch the baseline.

One flake to know about, seen once in four complete runs on this branch: DeltaCDCStreamWithCatalogManagedBatch100Suite#maxFilesPerTrigger with Trigger.AvailableNow respects read limits was reported as a regression by run 33044870485 and by none of 33043547622, 33141742435 or 33148690783. flaky-tests.txt has no active entries, so if it recurs it will red a shard.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude claude-opus-5

Copilot AI lite review requested due to automatic review settings August 27, 2026 04:49
@github-actions github-actions Bot added the INFRA label Aug 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an Actions concurrency edge case in delta_spark_ut.yml where any issue_comment event (including normal review comments) could land in the same concurrency group as a PR’s in-flight Delta Spark UT run and cancel it due to cancel-in-progress: true. The change makes cancellation conditional so only a real /delta-test command comment can cancel an in-flight run for that PR, preserving the intended “new push cancels old run” and “explicit rerun command cancels old run” behavior.

Changes:

  • Replace unconditional concurrency.cancel-in-progress: true with a conditional expression that only enables cancellation for real /delta-test comment commands.
  • Keep the concurrency group key unchanged, but align the cancellation condition with the existing jobs.delta-test-requested.if logic to avoid unintended cancellations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@felipepessoto

felipepessoto commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Thanks for fixing it @LuciferYang. I wonder if the same happens to .github/workflows/velox_backend_ansi.yml. If I start a /ansi-test, any following comment in the same PR would cancel it?

I think we don't have a good way to tests your changes before merging it. But it looks good to me

@github-actions

Copy link
Copy Markdown

🔄 ANSI mode analysis started by @felipepessoto. View run

@felipepessoto

felipepessoto commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Test checking if ansi is cancelled

UPDATE: yes it does: https://github.com/apache/gluten/actions/runs/33043165559

@github-actions

Copy link
Copy Markdown

ANSI Mode Test Analysis Report (Spark 4.1)

Note

Expression-level ANSI mode offload coverage analysis.
Test config: spark.sql.ansi.enabled=true, spark.gluten.sql.ansiFallback.enabled=false.

  • Passed (🟢): Velox correctly handles ANSI semantics
  • Fallback (🔴): Expression falls back to Spark execution, needs ANSI support in Velox
  • Failed (🟡): Velox executes but ANSI error behavior differs from Spark, needs exception handling fix

ANSI Offload suites: 0 tests, 0 records | Other suites: 0 tests

ANSI Offload

Overview (ANSI Offload Expression Records)

Classification Count %

Conditioning cancel-in-progress instead would leave the run pending until the
in-flight suite ends, up to timeout-minutes: 350, showing as an unresolved check
the whole time. Taking it out of the group keeps today's behaviour for the two
cases the group exists for and removes only the collateral cancellation.
Copilot AI review requested due to automatic review settings August 27, 2026 05:45
@felipepessoto

Copy link
Copy Markdown
Contributor

Created the issue #12908 in case you are interested in fixing it as well

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Keeps the group name byte-identical to main for every other trigger, so PRs open
at merge time do not land in a window where a push run and a /delta-test run sit
in different groups and stop superseding each other.
Copilot AI review requested due to automatic review settings August 27, 2026 06:00
@LuciferYang LuciferYang changed the title [MINOR][CI] Only let a real /delta-test comment cancel an in-flight Delta Spark UT run [MINOR][CI] Stop an unrelated PR comment from cancelling an in-flight Delta Spark UT run Aug 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 27, 2026 06:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Same shape as delta_spark_ut.yml: any comment on a PR joined the ANSI group and
cancelled an in-flight /ansi-test or /ansi-analyze run, then skipped in
check-comment. Fixes the case reported in apache#12908.
Copilot AI review requested due to automatic review settings August 27, 2026 08:42
@LuciferYang LuciferYang changed the title [MINOR][CI] Stop an unrelated PR comment from cancelling an in-flight Delta Spark UT run [MINOR][CI] Stop an unrelated PR comment from cancelling in-flight Delta Spark UT and ANSI runs Aug 27, 2026
@LuciferYang

Copy link
Copy Markdown
Contributor Author

Good catch, thanks. Fixed in this PR too, at .github/workflows/velox_backend_ansi.yml, and the title now covers both workflows.

I took the second of your two options for both files, for the reason you gave: a unique group fully isolates the unrelated comment, while conditioning cancel-in-progress only downgrades the damage. Runs in one group never overlap, so with cancellation off the comment run does not proceed alongside the ANSI run, it sits pending until that run finishes and then skips anyway, holding the group's single pending slot in the meantime. I had it that way first and replaced it.

The suffix is appended only when the comment is not a command, so for workflow_dispatch, for pull_request and for a real /ansi-test or /ansi-analyze the group name stays byte-identical to what main has today. That matters beyond tidiness: pull_request runs use the workflow file from the PR head while issue_comment runs always use the default branch's, so renaming the group for every trigger would have put PRs that are already open into a window where a push run and a command run sit in different groups and stop superseding each other. Appending nothing in those cases avoids it.

The negated condition is check-comment's if copied verbatim, so the grouping and the gate cannot disagree. That includes its contains test, which means a comment merely mentioning /ansi-test counts as a command in both places. Whether that test should be tightened to a first-token match, the way delta-test-requested does it, seemed like a separate question from this one, so I left it alone.

Three properties are checkable without running anything, and I verified them for both files by parsing the YAML rather than reading it: cancel-in-progress is still true, removing the appended ${{ }} segment leaves a group string equal to main's, and the text inside !( ... ) equals the gate job's if after collapsing whitespace.

You are right that neither fix can be exercised before merging. issue_comment only ever runs the default branch's copy of a workflow, which is also why your reproduction ran main's unfixed version.

@github-actions

Copy link
Copy Markdown

🔄 ANSI mode analysis started by @LuciferYang. View run

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Two complete eight-shard runs on this branch report the same 32 now-passing
tests, byte-identical lists, with Stale and Skipped both zero. Deleted by hand
rather than committing the regenerated artifact, which also replaces the
hand-written header with an auto-generated stub.
Copilot AI review requested due to automatic review settings August 28, 2026 04:24
@LuciferYang LuciferYang changed the title [MINOR][CI] Stop an unrelated PR comment from cancelling in-flight Delta Spark UT and ANSI runs [MINOR][CI] Stop PR comments cancelling Delta/ANSI runs, refresh Delta baseline Aug 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment on lines 64 to 68
org.apache.spark.sql.delta.ConvertToDeltaSQLSuite#external tables use correct path scheme
org.apache.spark.sql.delta.ConvertToDeltaScalaSuite#external tables use correct path scheme
org.apache.spark.sql.delta.DeleteMetricsSuite#delete-metrics: delete one row per file - Partitioned = false, cdfEnabled = false
org.apache.spark.sql.delta.DeleteMetricsSuite#delete-metrics: delete one row per file - Partitioned = false, cdfEnabled = true
org.apache.spark.sql.delta.DeltaAllFilesInCrcSuite#test all-files-in-crc verification failure also triggers and logs incremental-commit verification result
Copilot AI review requested due to automatic review settings August 28, 2026 06:39
@LuciferYang LuciferYang changed the title [MINOR][CI] Stop PR comments cancelling Delta/ANSI runs, refresh Delta baseline [MINOR][CI] Stop unrelated PR comments from cancelling Delta and ANSI runs Aug 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment on lines 151 to +159
# Cancel older `pull_request` and `/delta-test` runs for the same PR.
#
# `issue_comment` fires on every comment posted on an issue or a PR, and
# `github.event.issue.number` is that issue's number, which for a comment on a
# PR is the PR's own number. Such a run would therefore join the PR's group and
# cancel an in-flight suite while `delta-test-requested` below skips it, losing
# the suite and replacing it with nothing. Give only that case a group of its
# own, so every other trigger keeps the exact group name it has today. The
# negated condition is that job's `if` verbatim.
@LuciferYang

Copy link
Copy Markdown
Contributor Author

cc @jackylee-ch CI paased

@jackylee-ch jackylee-ch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jackylee-ch
jackylee-ch merged commit 3a305e4 into apache:main Aug 28, 2026
19 checks passed
@LuciferYang
LuciferYang deleted the delta-ut-comment-concurrency branch August 28, 2026 10:49
@LuciferYang

Copy link
Copy Markdown
Contributor Author

Thank you @jackylee-ch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] Unrelated PR comments can cancel in-flight ANSI runs

4 participants